Skip to content

fix: type issues with Chart save methods.#3934

Merged
mattijn merged 3 commits intovega:mainfrom
alec-bike:chart-type-fix
Jan 7, 2026
Merged

fix: type issues with Chart save methods.#3934
mattijn merged 3 commits intovega:mainfrom
alec-bike:chart-type-fix

Conversation

@alec-bike
Copy link
Copy Markdown
Contributor

@alec-bike alec-bike commented Dec 31, 2025 •
edited
Loading

Chart.save() methods trigger type-check warnings with ty. This PR adds some type annotations to avoid these warnings.

Resolves #3870.

To test run ty:

uvx ty check altair/vegalite/v6/api.py
uvx ty check altair/utils/save.py

uv run task test reveals no change in test results compared to main.

Notes:

  • many more type issues still remain in these 2 files
  • (reverted) replacing Union with | requires min python version 3.10

@alec-bike
Copy link
Copy Markdown
Contributor Author

alec-bike commented Jan 4, 2026

@mattijn could you please approve the workflow for the latest commit? The original commit failed due to incompatibility with python 3.9 so I reverted that change and rebased.

@mattijn
Copy link
Copy Markdown
Contributor

mattijn commented Jan 4, 2026

Approved the workflow! Looks good! And great that you work on this PR🙌.

I've one question: do we need the change regarding this line:

Before

values: Any = Undefined

After

values: dict[str, Any] | list | InlineDataset | None = None

There have been long discussions in the past on the difference between None and Undefined (also type-wise) and at that time end up with the conclusion that there is a difference between these.

@alec-bike
Copy link
Copy Markdown
Contributor Author

alec-bike commented Jan 4, 2026

Thanks for approving, the PR passes now. Let me look more into Undefined vs None and revert or update as needed.

@alec-bike
Copy link
Copy Markdown
Contributor Author

alec-bike commented Jan 5, 2026

Neither ty nor basedpyright complained about the (original) use of Undefined so I reverted that change. Might be worth adding a unit test to illustrate the difference.

@mattijn
Copy link
Copy Markdown
Contributor

mattijn commented Jan 5, 2026

Nice! Can this PR be moved out of draft status?

@alec-bike alec-bike marked this pull request as ready for review January 5, 2026 15:33
@alec-bike
Copy link
Copy Markdown
Contributor Author

alec-bike commented Jan 5, 2026 •
edited
Loading

Done. PR is live and ready to review.

@mattijn mattijn merged commit 846f091 into vega:main Jan 7, 2026
15 checks passed
@mattijn
Copy link
Copy Markdown
Contributor

mattijn commented Jan 7, 2026

Thanks @alec-bike! Looks great

@alec-bike alec-bike mentioned this pull request Jan 7, 2026
Closed
@alec-bike alec-bike deleted the chart-type-fix branch January 7, 2026 17:06
alec-bike added a commit to alec-bike/altair that referenced this pull request Jan 11, 2026
@alec-bike
Revert "fix: type issues with Chart save methods. (vega#3934)"
89d58dd 
This reverts commit 846f091.
alec-bike added a commit to alec-bike/altair that referenced this pull request Jan 11, 2026
@alec-bike
fix: type issues with Chart save methods. (vega#3934)
70e4cac 
* Fix unknown dict type issues.

* Ignore some type errors in save.

* Revert typing as None over Undefined.
alec-bike added a commit to alec-bike/altair that referenced this pull request Jan 11, 2026
@alec-bike
fix: type issues with Chart save methods. (vega#3934)
0c84c51 
* Fix unknown dict type issues.

* Ignore some type errors in save.

* Revert typing as None over Undefined.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Partially unknown type for some Chart methods.

2 participants

@alec-bike @mattijn